Cleanup ScriptService & friends in preparation for #6418#9992
Cleanup ScriptService & friends in preparation for #6418#9992javanna merged 1 commit intoelastic:masterfrom
Conversation
javanna
added
:Core/Infra/Scripting
|
@javanna Just a reminder here for when the PR is merged to also fix :
in their I believe there will be changes to port there. I think also that some river plugins might not compile anymore (test part) as if IIRC we are doing some script tests there (couchdb, rabbitmq). |
|
HI @dadoonet not sure what changes you mean that we should port there. In my mind these changes shouldn't break anything in scripting plugins, they shouldn't access the |
There was a problem hiding this comment.
Probably not something for this PR but we now have a convention of using script for inline, script_file for file and script_id for indexed scripts (using the ScriptParameterParser to maintain the consistency). Should we follow that convention here? I can see we may want to keep the inline as query but maybe we should also support script for inline? and file and id can probably be replaced with script_file and script_id?
There was a problem hiding this comment.
sounds good to me, patches welcome :)
|
@javanna Ignore me. I got confused by Github diff and thought you removed inner classes we are using in plugins. It's not the case. Sorry for the false alarm. |
|
I think you read my mind @dadoonet I wanted to do what you feared badly :) but I went for not breaking bw comp. I will probably do that on master only later on. |
javanna
force-pushed
the
enhancement/script_service_cleanup
branch
from
9216e5f to
34c0c64
Compare
|
LGTM |
javanna
force-pushed
the
enhancement/script_service_cleanup
branch
from
34c0c64 to
5c30425
Compare
…#6418 - Added NAME constants for each script language, avoiding to repeat the same strings all over the place. - Simplified `compile` method signatures by removing a couple of variants. Note that all of these signatures are going to change again with elastic#6418 as in order to compile/execute a script the caller will need to specify which operation is attempting to execute the script, info that will be provided as an additional mandatory argument. - Removed double call to ScriptService#verifyDynamicScripting for every indexed or dynamic script. - Decreased ScriptService inner classes visibility to private (CacheKey, IndexedScript, ApplySettings) - Moved ScriptService inner classes to the bottom of the class, I think it makes it more readable. - Resolved some compiler warnings Closes elastic#9992
javanna
force-pushed
the
enhancement/script_service_cleanup
branch
from
5c30425 to
521ac7f
Compare
- Added NAME constants for each script language, avoiding to repeat the same strings all over the place. - Simplified `compile` method signatures by removing a couple of variants. Note that all of these signatures are going to change again with #6418 as in order to compile/execute a script the caller will need to specify which operation is attempting to execute the script, info that will be provided as an additional mandatory argument. - Removed double call to ScriptService#verifyDynamicScripting for every indexed or dynamic script. - Decreased ScriptService inner classes visibility to private (CacheKey, IndexedScript, ApplySettings) - Moved ScriptService inner classes to the bottom of the class, I think it makes it more readable. - Resolved some compiler warnings Closes #9992
|
@javanna It effectively breaks: See https://github.com/elastic/elasticsearch-river-couchdb/blob/master/src/main/java/org/elasticsearch/river/couchdb/CouchdbRiver.java#L145-145 The method signature changed. I need to fix that I guess. :) We just need to change Maps.newHashMap()to Maps.<String, Object>newHashMap() |
Caused by elastic/elasticsearch#9992 (comment) (cherry picked from commit ffe75f8)
|
Marked this as breaking. It breaks plugins that make use of scripting by depending on |
clintongormley
changed the title
4 participants
While working on #6418, I found myself cleaning some things up around scripting code, which I figured would be better to isolate on a different PR, to make the fine grained settings change easier to review.
compilemethod signatures by removing a couple of variants. Note that all of these signatures are going to change again with Allow fine-grained script settings #6418 as in order to compile/execute a script the caller will need to specify which operation is attempting to execute the script, info that will be provided as an additional mandatory argument.